Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apps sc & wc: private subnet as node-ips #1582

Merged
merged 11 commits into from
Sep 1, 2023

Conversation

robinelastisys
Copy link
Contributor

@robinelastisys robinelastisys commented May 9, 2023

What this PR does / why we need it:
#1453 #1677

Which issue this PR fixes (use the format fixes #<issue number>(, fixes #<issue_number>, ...) to automatically close the issue when PR gets merged):
fixes #1453
fixes #1677

Public facing documentation PR (if applicable)

Special notes for reviewer:
Ready for review.

Add a screenshot or an example to illustrate the proposed solution:

Checklist:

  • Added relevant notes to WIP-CHANGELOG.md
  • Proper commit message prefix on all commits
  • Updated the public facing documentation
  • Is this changeset backwards compatible for existing clusters? Applying:
    • is completely transparent, will not impact the workload in any way.
    • requires running a migration script.
    • will create noticeable cluster degradation.
      E.g. logs or metrics are not being collected or Kubernetes API server
      will not be responding while upgrading.
    • requires draining and/or replacing nodes.
    • will change any APIs.
      E.g. removes or changes any CK8S config options or Kubernetes APIs.
    • will break the cluster.
      I.e. full cluster migration is required.
  • Chart checklist (pick exactly one):
    • I upgraded no Chart.
    • I upgraded a Chart and determined that no migration steps are needed.
    • I upgraded a Chart and added migration steps.

Pipeline config (if applicable)
If you change some config options (e.g. add/rename variable or change the default value) you may need to update the config used by the pipeline in pipeline/config.

@robinelastisys robinelastisys self-assigned this May 9, 2023
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch 2 times, most recently from 8ae02ad to 52f1fdd Compare May 9, 2023 13:40
bin/update-ips.bash Outdated Show resolved Hide resolved
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch 2 times, most recently from c41629f to e222481 Compare May 9, 2023 14:41
@robinelastisys robinelastisys requested a review from aarnq May 9, 2023 14:45
@viktor-f
Copy link
Contributor

I think that this issue called for a more generic solution that allows us to preserve any given ip somehow. Something like this suggestion from the issue:

Alternatives would be to perhaps preserve comments to show why certain IPs differ or to ignore values with comments when considering the diff, perhaps both for added visibility.

I especially like the idea that it will not remove anything with a comment.

In addition to this I still want to keep your idea of always adding the private subnet (although as André said I'm not sure we should hard code it to 172.16.0.0/12).

@aarnq
Copy link
Contributor

aarnq commented May 10, 2023

I think that this issue called for a more generic solution that allows us to preserve any given ip somehow. ...

Yeah we had a discussion on Slack regarding it, and the new idea is that it will treat subnet CIDRs differently compared to individual IPs. And if it finds a configured CIDR it should keep it and ignore any IPs it finds that matches those CIDRs.

We might still want to add the comment approach to to cover individual IPs as well, though I'm unsure if we have great use of that for those IPs we manage with the script. 🤔

@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch 4 times, most recently from 5b9d8f4 to 7ce0db2 Compare May 30, 2023 07:51
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch 2 times, most recently from 88af902 to 2aea072 Compare May 30, 2023 12:05
@robinelastisys robinelastisys changed the title apps: private subnet as node-ips apps sc & wc: private subnet as node-ips May 30, 2023
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch 3 times, most recently from 9d1a66e to 5e2411b Compare May 31, 2023 13:59
@robinelastisys robinelastisys requested a review from Xartos June 1, 2023 07:51
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch from 5e2411b to 73a42c3 Compare June 2, 2023 06:59
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
@robinelastisys robinelastisys requested a review from Xartos June 9, 2023 09:03
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch 2 times, most recently from abb1fc3 to 74e90e2 Compare June 13, 2023 06:12
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch from 74e90e2 to 4674937 Compare June 21, 2023 06:48
@robinelastisys
Copy link
Contributor Author

@viktor-f @aarnq @Xartos

@robinelastisys robinelastisys requested a review from crssnd June 27, 2023 14:52
@aarnq
Copy link
Contributor

aarnq commented Jul 3, 2023

The update command seems to well now, though I think we need to work on the dry-run part as well. 😅

@aarnq
Copy link
Contributor

aarnq commented Jul 3, 2023

I propose that we try to hook into the process earlier by running this as a filter on the list of ips (rather than modify how the ips are written into the file, as then we don't need to do a similar thing for how we diff as well). And with that we should rework the update-ips script in general so we can have some common path for how we update and diff ips.

@robinelastisys robinelastisys linked an issue Aug 9, 2023 that may be closed by this pull request
@robinelastisys robinelastisys added the kind/improvement Improvement of existing features, e.g. code cleanup or optimizations. label Aug 9, 2023
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch 2 times, most recently from 00899c1 to 576d700 Compare August 10, 2023 09:50
Copy link
Contributor

@aarnq aarnq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked through the main flow but I will once these fundamentals are done.

bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
bin/update-ips.bash Outdated Show resolved Hide resolved
@@ -36,7 +36,7 @@ usage() {
echo " validate <wc|sc> validates config files" 1>&2
echo " providers lists supported cloud providers" 1>&2
echo " flavors lists supported configuration flavors" 1>&2
echo " update-ips <wc|sc|both> <update|dry-run> Automatically fetches and updates the IPs for network policies" 1>&2
echo " update-ips <wc|sc|both> <apply|dry-run> Automatically fetches and applies the IPs for network policies" 1>&2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
echo " update-ips <wc|sc|both> <apply|dry-run> Automatically fetches and applies the IPs for network policies" 1>&2
echo " update-ips <wc|sc|both> <apply|dry-run> Automatically fetches and updates the IPs for network policies" 1>&2

bin/ck8s Show resolved Hide resolved
@robinelastisys robinelastisys requested a review from Xartos August 24, 2023 08:54
@robinelastisys robinelastisys force-pushed the robinr/sc-node-update-ips-octivia branch from f68b4aa to 659a956 Compare September 1, 2023 08:41
@robinelastisys robinelastisys merged commit 94140bc into main Sep 1, 2023
@robinelastisys robinelastisys deleted the robinr/sc-node-update-ips-octivia branch September 1, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/improvement Improvement of existing features, e.g. code cleanup or optimizations.
Projects
None yet
4 participants